Reviewed 2 of 7 files at r1, 9 of 9 files at r2, all commit messages.
Reviewable status: 2 unresolved discussions, LGTM missing from assignees rpoyner-tri(platform),calderpg-tri (waiting on @jwnimmer-tri and @rpoyner-tri)

common/test/openmp_test.cc line 20 at r2 (raw file):

#else
constexpr bool kHasOpenmp = false;
// TODO(jwnimmer-tri) This should probably be a Drake helper function wrapper

fyi you can use or wrap equivalent helpers from common_robotics_utilities/openmp_helpers.hpp

common/test/openmp_test.cc line 34 at r2 (raw file):

  // Populate the storage, in parallel.
  constexpr int num_threads = 4;

Is there a reason for manually specifying the number of threads here and the number of cpus for the test, versus specifying OMP_NUM_THREADS=4 for the test and letting the omp parallel use all available threads? Or, alternatively something like CRU'sChangeOmpNumThreadsWrapper(4)/an explicit call to omp_set_num_threads(4)?

common/test/openmp_test.cc line 42 at r2 (raw file):

  }

  // Confirm how many threads were used.

btw This seems to be testing both that the expected number of threads were used, and that their thread numbers were the expected [0] or [0, 1, 2, 3]. If that is intended, perhaps the comment should reflect this. (If we only cared about number of threads run, presumably something more direct like

EXPECT_EQ(std::unordered_set<int>(outputs.begin(), outputs.end()).size(), kHasOpenmp ? num_threads : 1)

would be enough)

tools/workspace/csdp/package.BUILD.bazel line 57 at r2 (raw file):

    ],
    copts = [
        "-fno-openmp",

It looks like OpenMP was already disabled in these copts due to the use of longjmp

Reviewable status: 3 unresolved discussions, LGTM missing from assignees rpoyner-tri(platform),calderpg-tri (waiting on @calderpg-tri, @jwnimmer-tri, and @rpoyner-tri)

common/test/openmp_test.cc line 20 at r2 (raw file):

Yup, I had that in mind but should have written it down, too.

common/test/openmp_test.cc line 34 at r2 (raw file):

That would probably be an improvement. I'll take it for a spin; stay tuned.

common/test/openmp_test.cc line 42 at r2 (raw file):

Done.

tools/workspace/csdp/package.BUILD.bazel line 57 at r2 (raw file):

Done.

Reviewed 2 of 2 files at r3, all commit messages.
Reviewable status: 2 unresolved discussions, LGTM missing from assignees rpoyner-tri(platform),calderpg-tri (waiting on @jwnimmer-tri and @rpoyner-tri)

Reviewed all commit messages.
Reviewable status: 2 unresolved discussions, LGTM missing from assignees rpoyner-tri(platform),calderpg-tri (waiting on @jwnimmer-tri)

Reviewed 2 of 7 files at r1, 3 of 9 files at r2, 1 of 2 files at r3.
Reviewable status: 2 unresolved discussions, LGTM missing from assignee calderpg-tri (waiting on @jwnimmer-tri)

Reviewed 2 of 2 files at r4, all commit messages.
Reviewable status: 2 unresolved discussions, LGTM missing from assignee calderpg-tri (waiting on @jwnimmer-tri)

Reviewable status: 2 unresolved discussions, LGTM missing from assignee calderpg-tri (waiting on @calderpg-tri and @jwnimmer-tri)

common/test/openmp_test.cc line 34 at r2 (raw file):

Done.

I also reduced this test to just 2 threads, in case anyone ever runs it on a potato that doesn't have 4 cpus.

Reviewed 4 of 9 files at r2, 1 of 2 files at r3.
Reviewable status: 2 unresolved discussions, LGTM missing from assignee calderpg-tri (waiting on @calderpg-tri and @jwnimmer-tri)

Reviewed 2 of 2 files at r4, all commit messages.
Reviewable status: 1 unresolved discussion (waiting on @jwnimmer-tri)

platform

Reviewed 2 of 7 files at r1, 2 of 9 files at r2, 2 of 2 files at r4, all commit messages.
Reviewable status: 1 unresolved discussion (waiting on @jwnimmer-tri)

Reviewable status: complete! all discussions resolved, LGTM from assignees rpoyner-tri(platform),calderpg-tri,xuchenhan-tri(platform) (waiting on @calderpg-tri, @rpoyner-tri, and @xuchenhan-tri)

a discussion (no related file):

Given the current state of Clang in CI, there is no good way to deal with it in this PR. I'm going to add a GCC-specific hack to drake-ci shortly, so that we can at least have CI coverage of that starting point.